-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GTC-2794: Only owner or admin can use mutate dataset endpoints #512
GTC-2794: Only owner or admin can use mutate dataset endpoints #512
Conversation
…_protect_dataset_endpoints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be easier to read once Justin's changes are merged into feature/data_manager (since this current diff includes all of Justin's changes), but yes, your changes look good to me. Thanks for writing the fail and success test cases.
I assume you purposely skipped adding a test for update_dataset, since it is so similar to delete_dataset? Seems ok to not add tests for update_dataset.
async def get_owner( | ||
dataset: str = Depends(dataset_dependency), user: User = Depends(get_manager) | ||
) -> User: | ||
dataset_row: ORMDataset = await datasets.get_dataset(dataset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a """ .... """ comment line describing the function above this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, thanks! Also, I'm adding more tests.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## feature/data_manager #512 +/- ##
========================================================
+ Coverage 81.84% 81.87% +0.03%
========================================================
Files 125 125
Lines 5551 5562 +11
========================================================
+ Hits 4543 4554 +11
Misses 1008 1008
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, great test coverage!
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this introduce a breaking change?
Other information